@tautschnig @peterschrammel Please note that your suggestions to into opposite directions: Michael's means more copying, Peter wants less.

Please note that your suggestions to into opposite directions: Michael's means more copying, Peter wants less.

Well, yes, but on different bits of the code base. @peterschrammel's suggestions exclusively cover cases that require (at least so it seems) incremental construction; mine talks about cases where likely we could even add a const as the object is fully constructed in just one line of code.

Perhaps to make more fundamental progress on this:

  for(int i=0; i<2000000; i++)
  {
    irep_idt id;
    symbol_exprt s(id, bool_typet());
    exprt something;
    something.move_to_operands(s);
    //something.copy_to_operands(s);
  }

with move_to_operands: 0.59s
with copy_to_operands: 0.55s

This does not appear to require a recent compiler. The difference is actually difficult to measure (it's in the range of the noise). Thus, there is absolutely no reason to believe that moving ireps would be any faster than copying.

The key reason why the moving methods exist is that they did offer enormous benefit before the sharing was introduced. I am thus tempted to remove the .move() method, say for code_blockt as a starter. It's simply a distraction.

@kroening Thank you for doing this measurement! Radical as I am, may I propose the even bigger step of getting rid of move_to_operands altogether (in the long term, not in this PR)? And then we could also inline copy_to_operands...

Done; please take another look.

@tautschnig Indeed, will do a PR that deprecates move_to_operands().

Hang on, the move operations is surely only similar like this when there is no usage of the moved irept subsequently. For example,

irept i1 = something();
irept copy = i1;
copy.id() = "hello world";

This will cause a CoW break upon calling the non-const variant of .id(), whereas if we had moved i1 there would be no break. That sort of situation would surely produce a significant difference.

In the particular case discussed here we would have something like

block.move_to_operands(something);
...
replace_symbol(block, ...);

If something was copied then replace_symbol may cause a CoW break, whereas if it was moved then it won't.

@smowton Indeed, this assumes that the moved object isn't touched afterwards; in your example, I'd assume that 'something' isn't ever used after the move_to_operands. I would assume that to be the case across the code base, and would consider anything else to be a bug.

Right, but operations against the destination of the move are allowed, and if they take place before the moved-from handle has died then a needless CoW break will occur.

(in both my examples it's the target that's being altered, and the source handle just happens to still be alive)

@smowton To make progress on this, how about a variant of code_blockt::add that gets an rvalue reference? You'd then write

block.add(std::move(something));

in the scenario in which you would modify the added statement after adding.

Sure? Don't mind re: block.move(codet) and block.add(codet &&), I just don't want to needlessly throw away a useful opportunity to reduce CoW breaks

Ok, done!